Fix Claude source mode fallback on successful CLI login (#2403) - #2414
Fix Claude source mode fallback on successful CLI login (#2403)#2414Yuxin-Qiao wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1dc6bd8e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let metadata = self.store.metadata(for: .claude) | ||
| self.settings.setProviderEnabled(provider: .claude, metadata: metadata, enabled: true) | ||
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .auto |
There was a problem hiding this comment.
When a saved Claude token account contains a web/session cookie, this assignment changes an explicit web configuration to auto but leaves that token account selected. On the next app refresh, auto mode can pick the newly logged-in Claude Code OAuth/CLI credential before the selected account’s web cookie, while refresh publication still treats the selected token account as current and labels/caches that ambient snapshot under the saved account. In that configuration, users can see the CLI account’s usage under the selected web account; either clear/switch the selected token account or avoid auto-switching while a selected web-cookie account owns the refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9471a060c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let metadata = self.store.metadata(for: .claude) | ||
| self.settings.setProviderEnabled(provider: .claude, metadata: metadata, enabled: true) | ||
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .auto |
There was a problem hiding this comment.
When a user had explicitly selected Web but also has a Claude Admin API key saved or in the environment, this switch to Auto means the refresh triggered after a successful Claude Code login no longer uses the credential they just logged in with: ClaudeProviderDescriptor.resolveStrategies returns the admin API strategy immediately for Auto + admin key before reaching the OAuth/CLI planner. In that configuration the menu shows Admin API spend instead of the Claude Code account the login flow just authenticated; consider switching to a Claude Code-specific source or suppressing the admin-key auto shortcut for this post-login migration.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 31, 2026, 12:04 AM ET / 04:04 UTC. ClawSweeper reviewWhat this changesThis PR changes successful Claude Code logins from the saved Web usage source to Auto, adds regression coverage for post-login strategy planning, and adds retry handling for Swift toolchain installation in two Linux CI jobs. Merge readiness⛔ Blocked until real behavior proof is added - 4 items remain Keep this PR open: the latest branch addresses the previously raised enablement, CLI-fallback, and test-teardown concerns, but it still needs redacted real after-fix evidence of a Claude Code login followed by a successful persisted refresh before merge. Priority: P1 Review scores
Verification
How this fits togetherCodexBar’s Claude login flow saves provider enablement and a usage-source preference after the Claude CLI completes authentication. A subsequent refresh plans an Admin API, OAuth, CLI, or web strategy and sends the resulting usage state to the menu-bar provider display. flowchart LR
A[Claude Code login] --> B[Successful login result]
B --> C[Saved Claude settings]
C --> D[Refresh strategy planner]
D --> E[Admin API OAuth CLI or Web]
E --> F[Menu bar usage state]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the focused Auto migration and regression coverage, then add a redacted after-fix capture showing a real Claude Code login followed by a successful persisted Claude refresh for the affected configuration before merging. Do we have a high-confidence way to reproduce the issue? No: the branch has high-confidence source-level and deterministic test coverage for the strategy path, but no redacted real Claude Code login followed by a persisted successful refresh has been shown. Is this the best way to solve the issue? Yes, with proof still required: moving Web to Auto is the narrowest path that preserves OAuth-to-CLI fallback when Keychain reads are unavailable, and the latest branch adds coverage for the related planning cases. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ef86077e70a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
60e9173 to
987f6d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 987f6d696d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .oauth |
There was a problem hiding this comment.
When Claude is disabled—the setup used by both login-flow tests and a normal sign-in onboarding path—replacing the unconditional setProviderEnabled(..., enabled: true) call with this source-only update leaves ProviderConfig.enabled false. Changing claudeUsageDataSource does not alter enablement, so the flow reports success and posts a notification while Claude remains hidden and is not refreshed; the new isProviderEnabledCached assertions consequently fail. Restore provider enablement independently of the selected source.
Useful? React with 👍 / 👎.
| #expect(actions.contains { | ||
| $0.0 == "Re-login at claude.ai" && $0.1 == .loginToProvider(url: "https://claude.ai/") |
There was a problem hiding this comment.
After the successful flow changes this test's source from .web to .oauth, shouldOpenBrowserForWebSessionError rejects the source because it only accepts .auto or .web. Injecting a web unauthorized error therefore cannot produce the asserted “Re-login at claude.ai” action, so this test will fail once the earlier enablement assertion is repaired and does not validate the new routing behavior; assert the OAuth behavior or verify that the obsolete web action is absent instead.
Useful? React with 👍 / 👎.
… and add swiftly retry
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf2a3ef70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let metadata = self.store.metadata(for: .claude) | ||
| self.settings.setProviderEnabled(provider: .claude, metadata: metadata, enabled: true) | ||
| if self.settings.claudeUsageDataSource == .web { | ||
| self.settings.claudeUsageDataSource = .oauth |
There was a problem hiding this comment.
When “Disable Keychain access” or the no-prompt OAuth policy is enabled, claude auth login --claudeai can still succeed because it runs as an external CLI process, but this assignment then forces the provider into strict OAuth mode. ClaudeOAuthFetchStrategy cannot obtain the newly stored Keychain credential in that configuration, and its shouldFallback implementation only permits the working CLI strategy in .auto mode, so the refresh immediately fails despite the successful login. Keep a CLI fallback for this case rather than unconditionally selecting .oauth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c526a05781
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| account: fetcher.loadAccountInfo(), | ||
| updater: DisabledUpdaterController(), | ||
| preferencesSelection: PreferencesSelection(), | ||
| statusBar: .system) |
There was a problem hiding this comment.
On macOS test runs, this now constructs a controller against NSStatusBar.system for every source mode without calling releaseStatusItemsForTesting; the removed withStatusItemControllerForTesting helper deferred exactly that cleanup, and StatusItemController.deinit does not remove its status items. These iterations can therefore leave real status items registered and pollute later headless AppKit tests, so retain the helper or add equivalent deferred teardown.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
…#2403) - Route migrated web logins through Auto so OAuth failure can fall back to the CLI credential when Keychain reads are unavailable - Restore withStatusItemControllerForTesting teardown in the login-flow policy test - Add issue 2403 reproduction coverage for post-login strategy planning Co-authored-by: Cursor <cursoragent@cursor.com>
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
1 participant
Summary
Fixes #2403. When users logged into Claude via "Sign in with Claude Code..." (terminal CLI authentication), if
claudeUsageDataSourcewas previously set to.web(or defaulted to.web), CodexBar would immediately attempt to fetch usage using unauthenticated browser cookies on the subsequent refresh. This resulted in an immediate401 Unauthorizederror ("Sign in to claude.ai...") and reverted the Claude status item to disconnected immediately after a "successful" login.This PR updates
runClaudeLoginFlow()so that a successful terminal CLI login automatically updatesclaudeUsageDataSourcefrom.webto.auto, ensuring newly generated OAuth/CLI credentials are used immediately during refresh.Changes Made
claudeUsageDataSourcefrom.webto.autowhenrunClaudeLoginFlowcompletes with.success. Routing through Auto keeps the CLI fallback when OAuth Keychain reads are unavailable after a terminal login..websource mode switching to.autoupon successful CLI login.withStatusItemControllerForTestingteardown so each iteration releases its status items.resolveStrategiesForTestingseam for strategy-planning assertions.Verification Run
swift test --filter ClaudeLoginFlowTests✅swift test --filter ClaudeIssue2403ReproductionTests✅swift test --filter "AdaptiveRefreshTimerTests|DeferredMenuInteractionRefreshTailTests|StatusItemControllerShutdownTests|ClaudeIssue2403ReproductionTests|ClaudeLoginFlowTests"✅ (26 tests)swiftformat Sources Tests✅swiftlint --strict✅ (0 violations)Proof notes
ClaudeIssue2403ReproductionTestsdeterministically exercise the post-login Auto planning + fallback path in CI.swiftly installblocks now retry up to 5 times; this is scoped as a CI reliability improvement for flaky toolchain downloads.